Skip to content

python/swig test: add nvme.i consistency check and fix violations#3308

Open
martin-belanger wants to merge 1 commit intolinux-nvme:masterfrom
martin-belanger:check-nvme-i-consistency
Open

python/swig test: add nvme.i consistency check and fix violations#3308
martin-belanger wants to merge 1 commit intolinux-nvme:masterfrom
martin-belanger:check-nvme-i-consistency

Conversation

@martin-belanger
Copy link
Copy Markdown

python: add nvme.i consistency check and fix violations

Background

The SWIG interface file libnvme/libnvme/nvme.i exposes libnvme's internal C structs to Python. Struct fields are meant to be accessed through getter/setter accessor functions (e.g., libnvme_ctrl_get_name()) rather than by direct struct member access (e.g., ctrl->name). This is enforced through a combination of SWIG mechanisms:

  • Fields placed inside an inline %extend {} block cause SWIG to generate calls to the accessor functions instead of accessing the struct member directly.
  • Fields annotated !accessors:readonly in the private headers must be declared %immutable in nvme.i to prevent SWIG from generating a setter.

Keeping nvme.i consistent with the accessor annotations in private.h and private-fabrics.h is error-prone and easy to get wrong. Every time a new field is added to a !generate-accessors struct, or an annotation changes, nvme.i must be updated in sync — and there was previously no automated check to catch mistakes.

What this PR adds

libnvme/tools/check-nvme-i-consistency.py

A new static analysis script that validates nvme.i against the accessor annotations in the private headers. It is pure Python (stdlib only) and performs no compilation — it parses source files with regex + brace counting.

The script enforces the following rules:

Severity Rule Description
error Rule 1 A field annotated !accessors:readonly that is exposed in nvme.i must be declared %immutable.
error Rule 2 A field that has an auto-generated or bridged accessor must appear inside an inline %extend {} block, not as a direct struct member. Direct access bypasses the public accessor API.
error Rule 3 A field declared inside %extend {} must have a getter bridge (#define libnvme_STRUCT_FIELD_get ACTUAL_FUNC) whose target function is declared in at least one public header.
error Check 4 A bridge entry's target function is not found in any public header — stale or misspelled bridge.
error Check 5 A %immutable FIELD declaration in nvme.i refers to a field that does not exist in the corresponding C struct.
warning Check 6 A field has an accessor but is not exposed in nvme.i at all (may be intentional, but surfaces accidental omissions).
warning Check 7 A field's declared type in nvme.i differs from its type in the private header (e.g., char * vs const char *). A built-in equivalence table suppresses false positives from kernel-style typedefs (__u32unsigned int, __u8uint8_t, etc.).

The script bridges from two sources:

  • nvme-swig-accessors.i — auto-generated #define bridges for all accessor-generated functions
  • The %{ ... %} block in nvme.i — hand-written bridges for hand-written functions

The script exits with a non-zero status when errors are found (causing the meson test to fail) and with zero when only warnings are present. Errors are printed to stderr so they appear in meson-logs/testlog.txt on failure.

Meson test registration

The check is registered as a meson test in libnvme/libnvme/meson.build inside the if want_python block. It does not depend on the built SWIG module (pynvme_clib) — it is pure static analysis and runs quickly.

17/67 libnvme - check-nvme-i-consistency   OK   0.09s

Fixes to libnvme/libnvme/nvme.i

The new check immediately found 12 Rule 2 violations — fields with auto-generated getters that were exposed as direct struct members instead of through %extend {}. These have been fixed:

Struct Fields moved into %extend {}
libnvme_host hostnqn, hostid, hostsymname
libnvme_subsystem subsysnqn, model, serial, firmware, subsystype
libnvme_ctrl cntrltype, dctype, discovered
libnvme_ns nsid (new %extend {} block created)

Stale comments on cntrltype, dctype, and discovered claiming "no getter method in libnvme.map" have been removed — those getters exist and are correctly mapped in nvme-swig-accessors.i.

Testing

meson test -C .build 'libnvme - check-nvme-i-consistency'

The test passes with zero errors. Remaining warnings (Check 6, Check 7) reflect known intentional patterns in the current nvme.i and do not block the build.

Comment thread libnvme/tools/check-nvme-i-consistency.py Outdated
Add tools/check-nvme-i-consistency.py, a static analysis script that
verifies nvme.i stays consistent with the accessor annotations in
private.h and private-fabrics.h. It enforces several rules:

  Rule 1 (error):   !accessors:readonly fields exposed in nvme.i must
                    be declared %immutable.
  Rule 2 (error):   fields with auto-generated or bridged accessors
                    must be in %extend{} so SWIG invokes the getter
                    instead of accessing the struct member directly.
  Rule 3 (error):   fields in %extend{} must have a getter bridge
                    whose target is declared in a public header.
  Check 4 (error):  bridge target not found in any public header.
  Check 5 (error):  %immutable declaration for a field that does not
                    exist in the corresponding C struct.
  Check 6 (warning): field has an accessor but is not exposed in nvme.i.
  Check 7 (warning): field type in nvme.i differs from private header.

The check is registered as a meson test in libnvme/libnvme/meson.build
inside the 'if want_python' block. It performs pure static analysis and
does not depend on the built SWIG module.

Fix all Rule 2 violations found by the new check by moving the
offending fields into %extend{} blocks (using existing blocks where
possible, creating one for libnvme_ns). Remove stale comments on
cntrltype, dctype, and discovered that incorrectly claimed no getter
method was available.

Signed-off-by: Martin Belanger <[email protected]>
Assisted-by: Claude Sonnet 4.6 <[email protected]>
@martin-belanger martin-belanger force-pushed the check-nvme-i-consistency branch from b454be1 to 514511f Compare April 22, 2026 15:11
@igaw
Copy link
Copy Markdown
Collaborator

igaw commented Apr 23, 2026

The only nitpick I have this should be three patches: One for .gitingore, one for the nvme.i change and one for the check added. Rest looks good. Thanks!

@martin-belanger
Copy link
Copy Markdown
Author

@dwsuse - Actually, let's hold off on this PR. This may not be required following my proposal to generate most of the nvme.i directly from private.h and private-fabrics.h as per my email.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants